Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added x_transform for unevenly spaced data #142

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

vhu43
Copy link

@vhu43 vhu43 commented Mar 5, 2023

My proposed solution for #98

@codecov
Copy link

codecov bot commented Mar 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.23%. Comparing base (ea2f90f) to head (dc7fc65).

❗ Current head dc7fc65 differs from pull request most recent head 492d89a. Consider uploading reports for the commit 492d89a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
+ Coverage   98.20%   98.23%   +0.02%     
==========================================
  Files           5        5              
  Lines         223      226       +3     
==========================================
+ Hits          219      222       +3     
  Misses          4        4              
Flag Coverage Δ
unittests 98.23% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vhu43
Copy link
Author

vhu43 commented Mar 5, 2023

Sorry, accidentally did some wonky stuff with data_generator.py. Fixed this (to the best of my knowledge) and fixed a bug in the transform_x I wrote. I think this should be working

@arvkevi
Copy link
Owner

arvkevi commented Mar 6, 2023

Thanks for the PR @vhu43, I'll be able to spend more time with it later this week. I'm concerned because there is a syntax error on line 245 (missing parenthesis) and my CI didn't pick it up, so I'm looking into that issue. In the meantime, do you want to push a fix for the syntax error and get tests passing? When I ran them locally, a few failed.

@vhu43
Copy link
Author

vhu43 commented Mar 6, 2023

Sure thing, I fixed the error specified in it. I am working on some other analysis for my graduate degree, but will be available to fix implementation issues if needed.

@arvkevi
Copy link
Owner

arvkevi commented Apr 21, 2024

I think we need to update the threshold logic.

Do you want to try updating this code block, to be:

                knee = self.x[threshold_index]
                norm_knee = self.x_normalized[threshold_index]

Then, to get tests to pass you'd have to make these changes:

Change this line in test sine to this:

    expected_knees = [4.5, 4.9, 7.7, 8.1]

and change this line in test logistic
to this:

        online=False,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants